Adding 18_uruguay pre-alpha #10093
Adding 18_uruguay pre-alpha #10093
Conversation
There was a problem hiding this comment.
I'm sorry for the delay in this review, if I had looked at the PR closer to when it was opened I probably would have just closed it and asked to break it down into more manageable chunks.
If you want to address my comments piecewise, I recommend creating a new PR that has all of the config (maps/entity/etc constants, without any of the game logic or step classes, basically everything but new functions), and then separate PRs for separate features.
If you want to address the comments here instead of opening other PRs, it will make it easier for me to review the ongoing changes if you can keep the new individual commits focused on one feature and as small as possible.
Thanks for the contribution, I've heard great things about this game so it will be exciting to have a chance to play it on the site!
| module Round | ||
| class Nationalization < Engine::Round::Operating | ||
| def after_end_of_turn(action) | ||
| super |
There was a problem hiding this comment.
Are you planning on filling this out more in a later PR? If not it should be deleted.
| module Game | ||
| module G18Uruguay | ||
| module Round | ||
| class Stock < Engine::Round::Stock |
There was a problem hiding this comment.
Same thing as with the Nationalization round--it's fine to keep this here if it will be needed in a later PR, otherwise it should be removed and the base round class used instead.
|
|
||
| def check_and_apply_destination_bonus | ||
| corporation = current_entity.corporation | ||
| graph = @game.graph_for_entity(corporation) |
There was a problem hiding this comment.
Checking the graph on every lay tile action is expensive. Check out how 1822's destination token step works.
If the destination check does need to happen as part of the track step, this should at least be refactored so that the graph isn't rebuilt every time after the corporation has already received its second capitalization.
| return [] if @game.final_operating_round? | ||
|
|
||
| @round.loan_taken |= false | ||
| actions = super.map(&:clone) |
There was a problem hiding this comment.
Is there a reason why actions = super doesn't work? This applies to some other steps too.
|
|
||
| def process_take_loan(action) | ||
| entity = action.entity | ||
| @game.take_loan(entity, action.loan) unless @round.loan_taken |
There was a problem hiding this comment.
It would probably be best to raise a GameError if a loan already has been taken, to guarantee illegal actions can't be added to the DB.
|
|
||
| def reset_adjustable_trains!(_entity, routes) | ||
| routes.each do |route| | ||
| p 'Find a way to clear train from good' if train_with_goods?(route.train) |
There was a problem hiding this comment.
Could this be commented out until implemented, to avoid accidental logging to the console?
| p 'Find a way to clear train from good' if train_with_goods?(route.train) | |
| # p 'Find a way to clear train from good' if train_with_goods?(route.train) |
| end | ||
|
|
||
| def reset_train!(_train) | ||
| p 'Reset train!' |
There was a problem hiding this comment.
Could this be commented out until implemented, to avoid accidental logging to the console?
| visits.each do |visit| | ||
| return true if PORTS.include?(visit.hex.id) | ||
| end | ||
| false |
There was a problem hiding this comment.
Here's a more idiomatic loop you can use:
| visits.each do |visit| | |
| return true if PORTS.include?(visit.hex.id) | |
| end | |
| false | |
| visits.any? { |visit| PORTS.include?(visit.hex.id) } |
There was a problem hiding this comment.
The same kind of change could be applied to route_include_port?
|
|
||
| # LOANS | ||
| def init_loans | ||
| @loan_value = 100 |
There was a problem hiding this comment.
Could be pulled out to the top of the file as a constant instead of being an instance variable that is re-initialized every time this function is called?
| end | ||
|
|
||
| def attach_good_to_train(train, hex) | ||
| train.name += '+' + @game.class::GOODS_TRAIN + '(' + hex.id + ')' if hex |
There was a problem hiding this comment.
A lot of the code relating to train names is hard to read and understand, especially the regex parsing stuff. Is there another way that data about goods on the train could be stored, like an instance variable on the game class?
Pre-alpha release of 18_uruguay
The basic building stones should now be in place but there are still some graphics that needs to be updated
Before clicking "Create"
masterpinslabel if this change will break existing gamesdocker compose exec rack rubocop -adocker compose exec rack rakeImplementation Notes
Explanation of Change
Screenshots
Any Assumptions / Hacks